-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eligible storages for placement methods #578
Eligible storages for placement methods #578
Conversation
fb09ed8
to
dc73260
Compare
Pull Request Test Coverage Report for Build 3671
💛 - Coveralls |
6b986e3
to
748742b
Compare
bb575da
to
6e8ae11
Compare
e7cbf09
to
c3f4b2a
Compare
@gmcculloug Please review |
@@ -1,5 +1,5 @@ | |||
describe "SCVMM microsoft_best_fit_least_utilized" do | |||
let(:user) { FactoryBot.create(:user_with_group) } | |||
let(:user) { FactoryBot.create(:user_with_group, :settings => {:display => {:timezone => 'UTC'}}) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are actually doing eligible_resources
worflow, it is required to get the user timezone
. You can see the get_timezone
method here: https://github.com/ManageIQ/manageiq/blob/master/app/models/user.rb#L231. I was choosing from 3 options there. 1) override this method with allow and return rspec methods 2) set the user timezone inside the config 3) set the server timezone. I picked the 2nd option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you point me to where the timezone
is required by eligible_resources
? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the error output without setting the timezone. You can see the whole path here:
NoMethodError:
undefined method `server_timezone' for nil:NilClass
# /home/pkomanek/manageiq/manageiq/app/models/mixins/timezone_mixin.rb:21:in `server_timezone'
# /home/pkomanek/manageiq/manageiq/app/models/user.rb:219:in `get_timezone'
# /home/pkomanek/manageiq/manageiq/lib/rbac/filterer.rb:220:in `search'
# /home/pkomanek/manageiq/manageiq/lib/rbac/filterer.rb:146:in `search'
# /home/pkomanek/manageiq/manageiq/lib/rbac.rb:3:in `search'
# /home/pkomanek/manageiq/manageiq/lib/rbac/filterer.rb:441:in `filtered'
# /home/pkomanek/manageiq/manageiq/lib/rbac/filterer.rb:150:in `filtered'
# /home/pkomanek/manageiq/manageiq/lib/rbac.rb:11:in `filtered'
# /home/pkomanek/manageiq/manageiq/app/models/miq_search.rb:43:in `filtered'
# /home/pkomanek/manageiq/manageiq/app/models/miq_request_workflow.rb:815:in `process_filter'
# /home/pkomanek/manageiq/manageiq/app/models/miq_request_workflow.rb:1056:in `allowed_hosts_obj'
# /home/pkomanek/manageiq/manageiq/app/models/miq_provision_virt_workflow.rb:190:in `allowed_hosts_obj'
# /home/pkomanek/manageiq/manageiq/app/models/miq_request_workflow.rb:1088:in `allowed_hosts'
# /home/pkomanek/manageiq/manageiq/app/models/mixins/miq_provision_mixin.rb:82:in `block in eligible_resources'
# /home/pkomanek/manageiq/manageiq/app/models/mixins/miq_request_mixin.rb:158:in `workflow'
# /home/pkomanek/manageiq/manageiq/app/models/mixins/miq_provision_mixin.rb:73:in `eligible_resources'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:333:in `public_send'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:333:in `block in object_send'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:352:in `ar_method'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:362:in `ar_method'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:331:in `object_send'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/service_models/mixins/miq_ae_service_miq_provision_mixin.rb:46:in `eligible_resources'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/service_models/mixins/miq_ae_service_miq_provision_mixin.rb:9:in `block (2 levels) in expose_eligible_resources'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:352:in `ar_method'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:362:in `ar_method'
# /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/service_models/mixins/miq_ae_service_miq_provision_mixin.rb:8:in `block in expose_eligible_resources'
# ./content/automate/ManageIQ/Infrastructure/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized.rb:42:in `best_fit_least_utilized'
# ./content/automate/ManageIQ/Infrastructure/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized.rb:17:in `main'
# ./spec/content/automate/ManageIQ/Infrastructure/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized_spec.rb:171:in `block (4 levels) in <top (required)>'
[MiqHashStruct.new(:id => host1.id, :evm_object_class => host1.class.base_class.name.to_sym), | ||
MiqHashStruct.new(:id => host2.id, :evm_object_class => host2.class.base_class.name.to_sym)] | ||
end | ||
let(:host_struct) { [host1, host2, host4] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
host_struct
is changed from array of MiqHashStruct to array of AR objects. Is this by purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look into the previous spec state, there was a hook, where were the #eligible_storages
and #eligible_hosts
methods overridden to return array of specific service models. We changed the automate method to use eligible_storages
instead of writable_storages
method and the filtering of the storages is now executed by eligible_resources
workflow. In this case I had to change the host_struct
, since it is no more used as the result of hookedeligible_hosts
, but in the #find_all_ems_of_type
, which is used inside the #allowed_hosts
as a part of the eligible workflow. With this change I am able to test the filtering.
@@ -31,7 +35,7 @@ | |||
end | |||
|
|||
# Set host and storage | |||
prov.set_host(host) if host | |||
host ? prov.set_host(host) : prov.set_option(:placement_host_name, [nil, nil]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but I suggest you move the prov.set_option(:placement_host_name, [nil, nil])
logic into a method named clear_host
which would make this easier to read.
host ? prov.set_host(host) : clear_host(prov)
...
def clear_host(prov)
prov.set_option(:placement_host_name, [nil, nil])
end
@@ -53,7 +57,7 @@ def best_fit_least_utilized | |||
end | |||
|
|||
# Set host and storage | |||
request.set_host(host) if host | |||
host ? request.set_host(host) : request.set_option(:placement_host_name, [nil, nil]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the same clear_host
technique here.
c3f4b2a
to
3aa946d
Compare
Some comments on commits pkomanek/manageiq-content@150229d~...3aa946d spec/content/automate/ManageIQ/Infrastructure/VM/Provisioning/Placement.class/methods/vmware_best_fit_least_utilized_spec.rb
|
Checked commits pkomanek/manageiq-content@150229d~...3aa946d with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
@pkomanek @gmcculloug can this be |
Hi @simaishi, yes, it can be ivanchuk/yes. |
…st_fit_least_utilized Eligible storages for placement methods (cherry picked from commit 957bbd9) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1805979
Ivanchuk backport details:
|
Changing the
host.writable_storages
torequest.eligible_storages
for ManageIQ placement methods to avoid getting the RBAC error:"not an eligible resource for this provisioning instance"
.Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1791390